I'm generally not sure about what this test is asserting. But this _updateNonPrimary(false) will always hit the short circuit return at the beginning of the function because _updatesRunning will always be true. It wont queue the updateAll either since the inflight is manually set to be active & the updateAll arg is false.

I get the test assertion now, but the _updateNonPrimary(false) doesn't exercise any code paths still. I assume this is to emulate how either a data message or an upgrade might have retriggered the _updateNonPrimary() call.

Probably can be removed given the inflight._active++ is the main thing being tested and shows that something is inflight which normally would interrupt the drain without the drain variable storing the original intent to drain.

_updateNonPrimary(false) does return early, but first sets _updatesQueued = true. That is separate from _inflight._active++: the test covers both new inflight work and another update being queued during the yield. Removing the call would lose coverage of the queued-update race. I clarified this in an inline comment 2407590

Didn't think about _updatesQueued being set, but it doesn't matter in this case because the drain keeps the inner loop going. The _updatesQueued makes the outer loop go another time, but it does no work because it isnt set to drain by then.

If we want to cover this additional loop in the test case, then we should assert the value of updates at the end. Naively I thought this would be the number of batches from one pass (Math.ceil(totalLength / 64)) plus the additional pass, but I didn't think about how the checkedSinceResolve is updated once a range is resolved. So the actual assertion looks like the following:

  const singlePass = Math.ceil(totalLength / 64)
  const repassBecauseOfResolving = Math.ceil(availableStart / 64)
  const firstPassBatches = singlePass + repassBecauseOfResolving
  t.is(updates, firstPassBatches + 1, 'does a second pass because _updateNonPrimary called mid update')

Does point out that the work done during a drain is not optimal but since this covers the case where ranges are added, I think it's fine as is. A more efficient approach can be part of refactoring seeks as well.

Also as an aside, the .catch(noop) isn't necessary right? _updateNonPrimary isn't allowed to throw.

My gut feeling says no about adding extra assertion here for now, since what we are testing that the queued update must not interrupt the current drain, and the extra assertion propose would make it overly strict towards future changes. I did the change fixing the noop catch though.

Ok then there is no purpose to the _updateNonPrimary call correct? It causes the outer loop to trigger again (by setting _updatesQueued) but the assertions don't cover that without the above addition. Without an assertion, it should be removed as well as the eventFlush() (and inflight._active = 0 could be too).

I could see either way as fine, as they essentially one of two scenarios:

1. Testing that drain fully drains even when inflight blocks are sent when yielding the loop.
2. The same as above, with the additional coverage of another update queued.